fix(Card): updated markup for actionable cards #738
Conversation
| @@ -0,0 +1,9 @@ | |||
| import { JSXElement } from "estree-jsx"; | |||
|
|
|||
| export function nodeIsComponentNamed(node: JSXElement, componentName: string) { | |||
There was a problem hiding this comment.
@adamviktora seems like you and I built similar helpers (this from you, checkMatchingJSXOpeningElement in the nodeMatches file from me)? Wdyt about us combining them into a single rule + if so, any preference on naming?
There was a problem hiding this comment.
We can keep yours checkMatchingJSXOpeningElement, it is more general and we already have it as part of the initial general rule. I can refactor the two rules that are using this nodeIsComponentNamed to use checkMatchingJSXOpeningElement and then delete this helper.
...-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.ts
Show resolved
Hide resolved
...-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-pf-codemods/src/rules/helpers/removePropertiesFromObjectExpression.ts
Outdated
Show resolved
Hide resolved
...in-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.test.ts
Outdated
Show resolved
Hide resolved
| }, | ||
| // Passed as a variable reference | ||
| { | ||
| code: `import { Card, CardHeader } from '@patternfly/react-core'; const obj = {name: 'Test', selectableActionId: 'Id'}; <Card isClickable><CardHeader selectableActions={obj} /></Card>`, |
There was a problem hiding this comment.
I have more of a philosophical question to this - whether it is OK to remove / modify consumers variables. Because they might be using these variables for other stuff too. They shouldn't use them for multiple purposes, and they probably aren't, but it is technically possible, so we might break some code by modifying them.
I know we also have other issues opened, where the goal is to modify stuff in the place of variable declaration - I agree it is more convenient for the consumers too after all. I am just afraid we might break something - but if we do, it probably means it was a badly written code, ... so I am ok with modifying the variables.
There was a problem hiding this comment.
That's actually a good point. In this particular case the variable may not be shared with anything other than a CardHeader component since it's such a specific object (though wouldn't be surprised). But for something like a prop that accepts a color, maybe that's a valid case where a variable is being used in other places (maybe something like a "color" variable that possibly shouldn't be updated if it's being used in other places).
If anything we can base it on a case-by-case basis, but worth keeping in mind when trying to apply a fix to a variable like this.
There was a problem hiding this comment.
cc @wise-king-sullyman in case you had any thoughts
...-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-pf-codemods/src/rules/helpers/getSpecifierFromImports.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-pf-codemods/src/rules/helpers/getLocalComponentName.ts
Outdated
Show resolved
Hide resolved
| const isIdentifier = property.key.type === "Identifier"; | ||
| const { computed } = property; | ||
|
|
||
| // E.g. const key = "key"; {[key]: value} |
There was a problem hiding this comment.
+1 for these examples in comments
| if ( | ||
| attribute.value.type === "JSXExpressionContainer" && | ||
| attribute.value.expression.type === "Identifier" | ||
| ) { | ||
| const scope = context.getSourceCode().getScope(attribute); | ||
| const variableDeclaration = getVariableDeclaration( | ||
| attribute.value.expression.name, | ||
| scope | ||
| ); | ||
|
|
||
| return variableDeclaration && variableDeclaration.defs[0].node.init; | ||
| } | ||
|
|
||
| if (attribute.value.type === "Literal") { | ||
| return attribute.value; | ||
| } | ||
| if ( | ||
| attribute.value.type === "JSXExpressionContainer" && | ||
| ["ObjectExpression", "MemberExpression"].includes( | ||
| attribute.value.expression.type | ||
| ) | ||
| ) { | ||
| return attribute.value.expression; | ||
| } |
There was a problem hiding this comment.
Nit, but since two of these three outcomes require the value type to be JSXExpressionContainer it could be refactored to simplify the logic a bit. Additionally refactoring to use a switch and deconstructing the expression out would also tidy things up a but.
Not a blocker though.
| { | ||
| message: | ||
| "The markup for clickable-only cards has been updated.Additionally, the `selectableActions.selectableActionId` and `selectableActions.name` props are no longer necessary to pass to CardHeader for clickable-only cards.", | ||
| // message: `The markup for clickable-only cards has been updated, now using button and anchor elements for the respective clickable action. The \`selectableActions.selectableActionId\` and \`selectableActions.name\` props are also no longer necessary for clickable-only cards. Passing them in will not cause any errors, but running the fix for this rule will remove them.`, |
There was a problem hiding this comment.
Did you intend to leave this in?
thatblindgeye
force-pushed
the
cardUpdate
branch
from
3d5d8eb to
2d2a870
Compare
Closes #736
A todo is to update the message logic so that it'll only spit out the prop reference that is actually present (vs now it just spits out both
nameandselectableActionIdprops even if only one is present).Also attempted a helper that could possibly be used in relation to #695